-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NativeAOT-LLVM] Build on Linux #2574
Conversation
5ca340e
to
68c98bc
Compare
@@ -1,8 +1,2756 @@ | |||
{ | |||
"name": "@microsoft/dotnet-runtime", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the changes in this file intentional - why do we need them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, reverted.
cc @dotnet/nativeaot-llvm |
eng/native/gen-buildsys.sh
Outdated
if [[ -n "$NATIVEAOT_CI_WASM_BUILD_EMSDK_PATH" ]]; then | ||
source $NATIVEAOT_CI_WASM_BUILD_EMSDK_PATH/emsdk_env.sh | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Windows equivalent is in eng/common/build.ps1
. Can we do the same with eng/common/build.sh
to keep the scripts similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, moved.
# Install Wasm dependencies on Linux: emscripten, LLVM, NodeJS. | ||
- ${{ if and(eq(parameters.hostedOs, 'linux'), and(eq(parameters.runtimeFlavor, 'coreclr'), eq(parameters.archType, 'wasm'))) }}: | ||
- script: $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-emscripten.sh $(Build.SourcesDirectory)/wasm-tools | ||
displayName: Install/activate emscripten | ||
- script: pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.ps1 -Configs ${{ parameters.buildConfig }} -CI | ||
workingDirectory: $(Build.SourcesDirectory)/wasm-tools | ||
displayName: Install/build LLVM | ||
- script: pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-nodejs.ps1 $(Build.SourcesDirectory)/wasm-tools | ||
displayName: Install NodeJS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to unify the both Windows and Linux on pwsh
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, removed the .cmd
- ${{ if and(eq(parameters.hostedOs, 'linux'), and(eq(parameters.runtimeFlavor, 'coreclr'), eq(parameters.archType, 'wasm'))) }}: | ||
- script: $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-emscripten.sh $(Build.SourcesDirectory)/wasm-tools | ||
displayName: Install/activate emscripten | ||
- script: pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.ps1 -Configs ${{ parameters.buildConfig }} -CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameters.buildConfig
can be Checked
, to the normalization of of Config
in install-llvm.cmd
needs to be moved into install-llvm.ps1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
eng/pipelines/runtimelab.yml
Outdated
@@ -59,7 +59,7 @@ extends: | |||
helixQueuesTemplate: /eng/pipelines/coreclr/templates/helix-queues-setup.yml | |||
buildConfig: debug | |||
platforms: | |||
# - linux_x64 | |||
- linux_x64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These linux_x64
jobs only test upstream code - I think we can do with just one to exercise our changes to the build scripts (say, only Release).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RIght, have left just Release.
@@ -0,0 +1,16 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can use the same powershell sharing strategy as with the the other scripts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, have deletes the .cmd/.sh and just left the ps1
Expand-Archive -LiteralPath "$InstallPath\$NodeJSZipName" -DestinationPath $InstallPath -Force | ||
$NodeJSExePath = "$InstallPath\$NodeJSInstallName\node.exe" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Expand-Archive
not work on Linux (it's a bit surprising)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, but doesn't support gz
which is gzip, or xz
which is some LZMA I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And those are our options for linux https://nodejs.org/dist/v9.9.0/
src/coreclr/build-runtime.sh
Outdated
@@ -171,6 +171,26 @@ if [[ -n "$__RequestedBuildComponents" ]]; then | |||
__CMakeTarget=" $__RequestedBuildComponents " | |||
__CMakeTarget="${__CMakeTarget// paltests / paltests_install }" | |||
fi | |||
|
|||
if [[ "$__CMakeTarget" == *"wasmjit"* ]]; then | |||
__ExtraCmakeArgs="$__ExtraCmakeArgs -DCLR_CMAKE_BUILD_LLVM_JIT=1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need __ExtraCmakeArgs
in addition to __CMakeArgs
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed this one, but is in now.
src/coreclr/jit/CMakeLists.txt
Outdated
find_package(LLVM REQUIRED CONFIG PATHS $ENV{LLVM_CMAKE_CONFIG} NO_DEFAULT_PATH) | ||
find_package(LLVM REQUIRED CONFIG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we will search for the system LLVM install too? That would not be correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, removed.
src/native/libs/build-native.sh
Outdated
@@ -60,15 +60,15 @@ source "$__RepoRootDir"/eng/native/build-commons.sh | |||
|
|||
# Set cross build | |||
if [[ "$__TargetOS" == browser ]]; then | |||
if [[ -z "$EMSDK_PATH" ]]; then | |||
if [[ -z "$EMSDK" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to just set EMSDK_PATH
to EMSDK
somewhere above s.t. these lines do not need to be modified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
src/tests/Common/tests.targets
Outdated
@@ -6,7 +6,7 @@ | |||
</ItemGroup> | |||
|
|||
<PropertyGroup> | |||
<TestAssemblyDir Condition="'$(TestAssemblyDir)' == ''">$(BaseOutputPathWithConfig)\tests\</TestAssemblyDir> | |||
<TestAssemblyDir Condition="'$(TestAssemblyDir)' == ''">$(BaseOutputPathWithConfig)\Tests\</TestAssemblyDir> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does upstream not run into the same problem (in other words - is this an upstream bug)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have reverted this one, it looked wrong, but wasn't the problem I thought it was.
Lots of little commits, but think that's all the comments above addressed. |
eng/common/build.sh
Outdated
host_arch='' | ||
target_os='' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these only required for NATIVEAOT_CI_WASM_BUILD_EMSDK_PATH
? It's not clear what they're for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its to avoid
/home/scott/github/runtimelab/eng/common/build.sh: line 245: host_arch: unbound variable
${host_arch-''}
might be another way to solve it, or make it a mandatory parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the only uses of host_arch
that I see look to be new (added in this change). My suggestion is delete them - or are they needed for something?
python ./emsdk.py install 3.1.47 | ||
|
||
./emsdk activate 3.1.47 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have install-emscripten.cmd
referenced in docs/using-nativeaot/prerequisites.md
and also docs/workflow/building/coreclr/nativeaot.md
, these need to be updated now.
Also, it is odd that the first line calls .py
directly while the second goes through the wrapper script.
[string]$InstallDir | ||
) | ||
|
||
New-Item -ItemType Directory -Force -ErrorAction SilentlyContinue -Path (Split-Path -Path $InstallDir -Parent) -Name (Split-Path -Path $InstallDir -Leaf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really need to be this elaborate?
I just tried:
/home/singleaccretion> New-Item /home/singleaccretion/test/xyz/dir -Force -ItemType Directory
And it created the nested directory as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its ok on Linux, but with a drive letter it fails. We could strip the drive letter and assume we are "in" that drive, but it probably isn't much easier to read.
PS C:\Users\scott> new-item -ItemType Directory -Force -Name c:/tmp/1/2
new-item : The given path's format is not supported.
At line:1 char:1
+ new-item -ItemType Directory -Force -Name c:/tmp/1/2
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (:) [New-Item], NotSupportedException
+ FullyQualifiedErrorId : System.NotSupportedException,Microsoft.PowerShell.Commands.NewItemCommand
PS C:\Users\scott> new-item -ItemType Directory -Force -Name /tmp/1/2
Directory: C:\Users\scott\tmp\1
Mode LastWriteTime Length Name
---- ------------- ------ ----
d----- 11/05/2024 16:54 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new-item -ItemType Directory -Force -Name c:/tmp/1/2
I think this should use -Path
. I tried:
PS C:\Users\Accretion\AppData\Local\Temp> new-item -Force -ItemKind Directory -Path C:\Users\Accretion\AppData\Local\Temp/xyz/rot/tot
Directory: C:\Users\Accretion\AppData\Local\Temp\xyz\rot
Mode LastWriteTime Length Name
---- ------------- ------ ----
d---- 13.05.2024 00:22 tot
displayName: Install NodeJS | ||
|
||
# Install Wasm dependencies on Linux: emscripten, LLVM, NodeJS. | ||
- ${{ if and(eq(parameters.hostedOs, 'linux'), and(eq(parameters.runtimeFlavor, 'coreclr'), eq(parameters.archType, 'wasm'))) }}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we collapse the Windows/Linux into one section now (will require putting Emscripten on the same pwsh
plan)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, done
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
New-Item -ItemType Directory -Force -ErrorAction SilentlyContinue -Name $InstallDir | ||
|
||
$ErrorActionPreference="Stop" | ||
|
||
Set-Location -Path $InstallDir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New-Item -ItemType Directory -Force -ErrorAction SilentlyContinue -Name $InstallDir | |
$ErrorActionPreference="Stop" | |
Set-Location -Path $InstallDir | |
$ErrorActionPreference="Stop" | |
New-Item -Path $InstallDir -ItemType Directory -Force | |
Set-Location -Path $InstallDir |
I think this ought to work...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, indeed that does.
displayName: Install/activate emscripten | ||
- script: call $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.cmd $(Build.SourcesDirectory)\wasm-tools $(Build.SourcesDirectory) ${{ parameters.buildConfig }} | ||
- script: pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.ps1 $(Build.SourcesDirectory)\wasm-tools $(Build.SourcesDirectory) ${{ parameters.buildConfig }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- script: pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.ps1 $(Build.SourcesDirectory)\wasm-tools $(Build.SourcesDirectory) ${{ parameters.buildConfig }} | |
- script: pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.ps1 $(Build.SourcesDirectory)/wasm-tools $(Build.SourcesDirectory) -Configs ${{ parameters.buildConfig }} -CI |
The cmake detection and cd $(Build.SourcesDirectory)/wasm-tools
need to be migrated from install-llvm.cmd
to install-llvm.ps1
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cmake detection, i.e. executing init-vs-env.cmd
as we do, doesn't seem necessary, and puts the CMakePath
at the end anyway so doesn't precede anything already in the path. It is possible to do from powershell, but would involve a wrapper cmd
to echo
the CMakePath
back to the powershell do you scoping of environment variables and the lack or an equivalent call
in powershell. Question is, do we really need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had assumed it was needed. If that's not the case - great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need cmake.exe
in the path, so have added this back, which means we need a cmd
for the windows side.
eng/common/build.sh
Outdated
host_arch='' | ||
target_os='' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the only uses of host_arch
that I see look to be new (added in this change). My suggestion is delete them - or are they needed for something?
"But the only uses of host_arch that I see look to be new (added in this change). My suggestion is delete them - or are they needed for something?" You are right, I don't actually need it here, but I will for |
My preference would be introduce them in the WASM change. In general, it looks unexpected to me that we need to change |
Remove wasm build options add cmake to path on windows Use more Powershell where possible Simplify New-Item use Feedback
OK, removed here. |
Was on a break last week, so did this with lots of trail and error, but have now rebased to remove that noise and is green. |
- ${{ if eq(parameters.archType, 'x64') }}: | ||
- ${{ if eq(parameters.osGroup, 'windows') }}: | ||
- script: call $(Build.SourcesDirectory)/eng/pipelines/runtimelab/set-cmake-path.cmd | ||
displayName: Set CMake path | ||
- script: pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.ps1 -CI -InstallDir $(Build.SourcesDirectory)/wasm-tools -Configs ${{ parameters.buildConfig }} | ||
displayName: Install/build LLVM | ||
- ${{ if eq(parameters.osGroup, 'linux') }}: | ||
- script: $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-pwsh.sh $(Build.SourcesDirectory)/wasm-tools | ||
displayName: Install Powershell 7 | ||
# Install LLVM for the win-x64 and linux-x64 build as it will build the clrjit for browser_wasm cross compilation | ||
- script: $(Build.SourcesDirectory)/wasm-tools/powershell7/pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.ps1 -CI -InstallDir $(Build.SourcesDirectory)/wasm-tools -Configs ${{ parameters.buildConfig }} | ||
displayName: Install/build LLVM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will install tools for host configurations (windows_x64
, linux_x64
) that do not actually target WASM. I think the LLVM stuff should be deleted for windows_x64
and Install Powershell 7
folded into the above block (under {{ if and(eq(parameters.runtimeFlavor, 'coreclr'), eq(parameters.archType, 'wasm')) }}:
) under if ne(parameters.hostedOs, 'windows')
(this way it will, hopefully, also work for macOS if/when that comes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, done. The Linux build isn't pulling in these tools with this change but that's fine, they will be with #2569
- script: $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-pwsh.sh $(Build.SourcesDirectory)/wasm-tools | ||
displayName: Install Powershell 7 | ||
# Install LLVM for the win-x64 and linux-x64 build as it will build the clrjit for browser_wasm cross compilation | ||
- script: $(Build.SourcesDirectory)/wasm-tools/powershell7/pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.ps1 -CI -InstallDir $(Build.SourcesDirectory)/wasm-tools -Configs ${{ parameters.buildConfig }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
install-pwsh.sh
sets PATH
. Does a plain pwsh $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.ps1 ...
not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that will work, yes. Its not actually exercised with this PR but should be fine.
displayName: Install/activate emscripten | ||
- script: call $(Build.SourcesDirectory)/eng/pipelines/runtimelab/install-llvm.cmd $(Build.SourcesDirectory)\wasm-tools $(Build.SourcesDirectory) ${{ parameters.buildConfig }} | ||
- script: call $(Build.SourcesDirectory)/eng/pipelines/runtimelab/set-cmake-path.cmd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this will need to be put under if eq(parameters.hostedOs, 'windows')
, but let's leave that for the next change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
This PR reenables the build of the product on Linux, including the LLVM JIT. Does not build the Wasm runtime or libraries on Linux (to be done in #2569 )
Fixes #1713